so you're saying it's possible to have a list of vulnerabilities where all entries are null?

It is, programmatically. It doesn't make much sense to have that, but there a few strategies I can think of:

1. Prevent the construction of a VulnerableCapability if the collection of vulnerabilities contains any nulls. This would require iterating over the collection upon construction to validate if any of the entries are null. With big lists this could take a performance hit for constructing many Packages with IVulnerable and I would not favor this.
2. Allow the collection to contain nulls because it's just a list of objects, but handle them gracefully so we essentially treat them as if they did not exist. This is what I've done in this PR and made that behavior explicit by adding a test around it and to ensure that it is handled gracefully.

Can you think of another way you'd suggest instead?

Yea, I think iterating through them to validate there aren't any nulls seems excessive. I guess I didn't realize we had been accounting for this in the old code too.

@jgonz120 We aren't accounting for it explicitly in the ViewModels today which presents a potential flaw. The goal here is to make this code more robust than what we had before and put it under test. By handling this edge case gracefully, we avoid any NRE's in case a null makes its way into the list at any point.

This file has #nullable enable, and the definition of the Vulnerabilities property doesn't allow nulls, so either the definition is wrong, or this not null check shouldn't be needed.

This file has #nullable enable, and the definition of the Vulnerabilities property doesn't allow nulls, so either the definition is wrong, or this not null check shouldn't be needed.

I see what you mean. Both the tests and the VulnerabilityCapability classes themselves have #nullable enable yet the Test allows a null value to be used. As such, it proves that with nullable enabled on the caller, we can still end up with nulls making their way into the list. While a test can show that it's possible, we should have code that handles this edge case and a test that proves it's being handled. If I wrote the test incorrectly though, please let me know how and I'll fix it!

This is great for performance, not sure if we are currently iterating. More context: https://learn.microsoft.com/en-us/nuget/api/vulnerability-info#vulnerability-page.

If this is not officially part of the contract, we shouldn't depend on it.

Correct, there's no way to guarantee the list will be ordered from any and all vulnerability sources, both current and future. This comment points out that the list is ordered. It's ordered in this class (see the setter) explicitly to avoid making assumptions about the data source. Does that help? Do you have any suggested edits?

It does, right? The list of known vulnerabilities for a package should be sorted in descending order of the max version of the version range.

Doesn't that talk about the way the version ranges are sorted and not the list of vulnerabilities?

Can you use more concrete types to avoid paying the enumerator costs?

Make this class immutable and avoid paid the cost of Any.
In general, I'd avoid properties that force a compute that's not cached.

Additionally, the current implementation of calculating the IsVulnerable property gets the Vulnerabilities property, which in turn uses LINQ's OrderByDescending API. Therefore, in order to determine if the current package is vulnerable, the code is going to allocate a list of equal or greater capacity to the _vulnerabilities enumerable, spend CPU cycles sorting it, then just check if there's a first item, without caring about the contents of it. The .Any() call causes an enumerator to be allocated on the stack, as well as the OrderByDescending call. Both of these methods pass in a predicate, and unless the C# compiler has improved since I last checked, they both cause heap allocations as well (could be avoided by creating class methods and passing it without using an anonymous function, but that makes the code less pretty).

Referring to Nikolche's other comment about using a concrete class, all these heap allocations could be avoided by using a concrete class, and making this property's body => _vulnerabilities.Count > 0

Additionally, the current implementation of calculating the IsVulnerable property gets the Vulnerabilities property, which in turn uses LINQ's OrderByDescending API. Therefore, in order to determine if the current package is vulnerable, the code is going to allocate a list of equal or greater capacity to the _vulnerabilities enumerable, spend CPU cycles sorting it, then just check if there's a first item, without caring about the contents of it.

The .Any() call causes an enumerator to be allocated on the stack, as well as the OrderByDescending call

This is not correct. The OrderByDescending is in the setter of the Vulnerabilities property, not in the getter. Therefore, it will only be sorted once when assigned privately, not upon each access.